-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🌱 KCP: Add current/desired objects to NotUpToDateResult & refactor object creation #12817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 KCP: Add current/desired objects to NotUpToDateResult & refactor object creation #12817
Conversation
/test pull-cluster-api-e2e-main |
9e97a69
to
c8b0bb8
Compare
/test pull-cluster-api-e2e-main |
c8b0bb8
to
48ab737
Compare
Signed-off-by: Stefan Büringer buringerst@vmware.com
48ab737
to
9dab202
Compare
/test pull-cluster-api-e2e-main |
/assign @fabriziopandini /cc @stmcginnis @furkatgofurov7 @alexander-demicev @lentzi90 (if you want to take a look) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a scary big refactor, might have been a good idea to break the various different refactors in distinctive commits.
|
||
// IsForJoin returns true if the KubeadmConfig is for a kubeadm join. | ||
func (c *KubeadmConfig) IsForJoin() bool { | ||
return c.Spec.JoinConfiguration.IsDefined() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubeadm allows you to pass all the kinds in the same yaml muilti doc. prints out a warning for init config if you pass it on join. that's how k-sigs/kind passes them.
is there a case where someone might define both the init and join configuration in the capi construct?
i'm trying to understand if this is the best method if a config is "for join".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our use case (KCP) the KubeadmConfigs we create for join always have joinConfiguration set and the ones for init don't have it. So this is the best way I'm aware of to figure out if a KCP KubeadmConfig is for a join.
is there a case where someone might define both the init and join configuration in the capi construct?
KCP would never do it, but other folks might do whatever they want. So probably this should not be a method on the API type and instead just a local func in the places where we use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to local private funcs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a bit over my head still, but for what it is worth, it looks reasonable to me.
A few minor notes below.
Thx everyone. All findings should be addressed. PTAL and +1 / resolve accordingly |
/test pull-cluster-api-e2e-main |
Nice, one step closer to in-place! |
LGTM label has been added. Git tree hash: 18ab74f34bec017d03c6892976e1848a6eb5152f
|
New changes are detected. LGTM label has been removed. |
/test pull-cluster-api-e2e-main |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #12291